Avoid memory leak in unit test driver #4249
Conversation
|
Scratch "gets selective valgrind runs of our unit tests clean for me" - I'm still seeing something else, and I'm not sure if it's just something I missed before or a regression from this PR. |
jwpeterson
left a comment
jwpeterson
left a comment
There was a problem hiding this comment.
I had some questions that could probably be cleared up if I read the cppunit docs, but I chose not to 🤷♂️
tests/driver.C
Outdated
| runner, rejects); | ||
| if (n_tests_added >= 0) | ||
| libMesh::out << "--- Running " << n_tests_added << " tests in total." << std::endl; | ||
| if (n_tests_added != -12345) |
There was a problem hiding this comment.
Minor comment, but since this is our own magic number that we now actually have to refer to, it might make sense to use libMesh::invalid_uint or some other named constant.
| if (n_tests_added >= 0) | ||
| libMesh::out << "--- Running " << n_tests_added << " tests in total." << std::endl; | ||
| if (n_tests_added != -12345) | ||
| owned_suite.reset(suite); |
There was a problem hiding this comment.
I guess I don't understand why we don't need to clean up suite when there's no tests added? From a surface level reading of the code it just looks like registry.makeTest() returns a dumb pointer whose lifetime we are expected to manage, and we were always leaking it before...
There was a problem hiding this comment.
This should clean up suite when there's no tests added. That'll return n_tests_added == 0, and 0 != -12345, so we put suite in our unique_ptr and it gets cleaned up.
|
There's definitely something weird going on here. This patch cleans up the leaked suite from If every test class was complaining then I'd be sure I'm trying to do a double-free here, once from the runner and then once from recursive destruction from the suite, but it's just No, wait, it's too much of a coincidence that the failing |
If we add only a subset of tests to the runner (via --re or --deny_re options), we're careful to move the other tests to a "rejects" suite so they'll get deleted there, but the "supertest", the suite holding all the other tests, wasn't getting deleted in those cases. This (on top of my earlier fixes, and using the MOOSE suppressions file for third-party library issues) gets selective valgrind runs of our unit tests clean for me.
roystgnr
force-pushed
the
dont_leak_test_suite
branch
from
f452222 to
0d0d8d1
Compare
|
I think I've managed to square the circle of "we can't add a test to a runner without passing ownership of the pointer to the runner" and "when we get a test from a suite there's no way to take ownership of it away from the suite": we get each test from the suite, we wrap it in our own I've added our |
3 participants
If we add only a subset of tests to the runner (via --re or --deny_re options), we're careful to move the other tests to a "rejects" suite so they'll get deleted there, but the "supertest", the suite holding all the other tests, wasn't getting deleted in those cases.
This (on top of my earlier fixes, and using the MOOSE suppressions file for third-party library issues) gets selective valgrind runs of our unit tests clean for me. (all-tests runs are clean either way)
This isn't urgent to merge; I still need to run through our examples to look for valgrind issues too. This tiny leak is only a problem because it upsets valgrind, and until we're sure we're ready to make that be a big deal (add a
--error-exitcode=option to our valgrind recipes), "do our unit tests upset valgrind" isn't a critical question.